-
Notifications
You must be signed in to change notification settings - Fork 565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compile Skia with Direct3D on Windows platform #2823
Conversation
These should be exposed by SkiaSharp: https://github.com/Wodsoft/upf/blob/master/src/UniversalPresentationFramework.Platforms.Win32/Win32RendererD3D12Context.cs#L291 |
@Gillibald How to update |
I think I may have broken the merge with my merge of the Metal PR. Sorry. But, this PR is shaping up very well! |
I think you found the generator in the utils directory. You should be able to just run this one on a Windows machine with VS installed: https://github.com/mono/SkiaSharp/blob/main/utils/generate.ps1 |
@mattleibow Yeah, I found it. Many documents are out of date or missing. I spent a lot of date to look up how to modify codes. Pipelines failed with |
ah, we have a mirror of all packages for reasons to prevent supply chain attacks. Let me mirror those packs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is green it seems. The samples are fail in some cases because the Tizen project was removed from the sln.
How much extra size is added to the binary?
binding/SkiaSharp/GRContext.cs
Outdated
@@ -63,6 +63,18 @@ public static GRContext CreateVulkan (GRVkBackendContext backendContext, GRConte | |||
} | |||
} | |||
|
|||
public static GRContext CreateDirect3D (GRD3dBackendcontext backendContext, GRContextOptions options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate myself so much... But this may need to be 3d with a lowercase d for consistency. So yuck to look at and I have no idea what I was thinking at the time. Sorry universe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw that every Direct3D library use uppercase D
, so I'm doing same with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but sadly if you look at my OpenGl
members I must have lost my mind. Everywhere it is uppercase-G-lowercase-l 😭
increase 1,773,537 bytes compare to 3.0.0-preview2.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very close, I added more comments on the native PR that is more about API shape and maybe an issue with native interop: mono/skia#121 (review)
Also, it would be great to get some sort of test to make sure that at least using these new types are not crashing because of some interop issue.
binding/SkiaSharp/GRContext.cs
Outdated
@@ -63,6 +63,18 @@ public static GRContext CreateVulkan (GRVkBackendContext backendContext, GRConte | |||
} | |||
} | |||
|
|||
public static GRContext CreateDirect3D (GRD3dBackendcontext backendContext, GRContextOptions options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but sadly if you look at my OpenGl
members I must have lost my mind. Everywhere it is uppercase-G-lowercase-l 😭
source/SkiaSharp.Direct3D/SkiaSharp.Direct3D.Vortice/GRDirect3DTextureInfo.cs
Outdated
Show resolved
Hide resolved
@mattleibow Why android and macos test run failed? |
I think there is a problem with the magic step that is supposed to use the correct build of skia. Just commit the submodule in this PR. Then the checkout of SkiaSharp will also pull the correct skia. |
For the tests, I think this will work - still installing tooling as I have a new laptop: 6cf79ea Hopefully it just works, I copied the context setup from the skia test files... seems simple enough, no idea. |
It's fail randomly I think. Sometime android failed and sometime macos failed or android and macos both failed. |
ah, yeah sorry. Those are a bit flakey right now. Sometimes the Android emulator does not boot, or the macOS test runner crashes. |
Maybe you can click |
@Kation are you able to write some tests? |
@mattleibow Workflow still report unauthorized. |
Yeah, the public pool is a bit full, so I am using an internal one. The build failed with the Nano Server checks saying there is no Direct3D available on Nano Server. I need to verify that, but for now I disabled D3D on Nano. So far, all the native builds looked green. Running again and the tests will tell us things hopefully. |
The internal pipeline finished and was all green. So, I think we should be able to merge this soon. I will give it another review and also try get the public builds working so folks can try out the artifacts. |
I will happily be guinea pig! |
/azp run |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
You should be able to download the |
Thanks for making those artifacts available! I'm afraid I hadn't looked too closely at the state of Skia's backend support and hadn't realized that the Ganesh set of backends included DX12 and not DX11. So, I was hoping that I could just rip out ANGLE and set up Skia contexts with DX11 instead. I don't think it should be a big deal to create a DX12 device and share the required textures/resources between the two, and probably still preferable to ANGLE for my usage. However, it'll take me a bit longer to spin something up with this than what I first thought... |
OK, I've got a working proof of concept using the D3D backend. Took me a little while to check the performance. Looks about 5-10% faster than the ANGLE build for our app, and I'll get to drop a couple of dependencies once this gets merged/released. Nice work @Kation! |
Very nice to hear! Are you able to share samples? I am no Direct-anything expert, so any samples will be great. I am wanting to add more things to the views as well as make better samples showing different backends. |
This is a good point. Thanks for making it here. However, the main aim of this PR (for me) is to merge the base APIs that people can use. If you (or anyone) are familiar with Silk.NET, then feel free to open a PR or issue with some code I can add to the repo. The way we are doing the nugets here is specifically to avoid a dependency lock-in, so the library specific code is in a separate nuget that can be deprecated/updated/changed at any time without breaking the core skia libraries. For example, to use Vortice, there will be a separate |
@mattleibow I'm not sure what value samples from my usage would add over the the unit tests that Kation added. I use different bindings, and most of the work I had to do was for D3D11/D3D12 interop, which I think is probably out of scope. D3D12 is a pretty complex beast - setting up Skia/SkiaSharp is the easy bit 😄 Suffice to say that I can create a GRContext and render to an SKSurface via GRBackendRenderTarget or GRBackendTexture just fine with this PR. @maw136 FWIW regarding bindings, I didn't use the SkiaSharp.Direct3D.Vortice package, just the base GRD3DBackendContext/GRBackendRenderTarget/GRD3DTextureResourceInfo. Using Silk.NET, CsWin32 or any other bindings directly with them should be essentially trivial. |
/azp run |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
OK, I think this PR is fianlly ready! Thanks for sticking with it for so long and everyone in the discussions. Going to merge and will let everyone know when it is available on the preview feed - typically in 3 hours. |
@mattleibow Pipeline still queue for running and report error, can we stop it? |
The pipeline is finished and the version is |
Description of Change
Compile Skia with Direct3D on Windows platform
Bugs Fixed
#2817
API Changes
None.
Behavioral Changes
None.
Required skia PR
mono/skia#121
PR Checklist